Skip to content

Added functions for ftp support #32

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

kebab01
Copy link

@kebab01 kebab01 commented Aug 24, 2023

  • Added 4 new functions for FTP support
  • Added a function to get the remaining space on the FS

Copy link
Collaborator

@PaulZC PaulZC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Thomas (@kebab01 ),
Excellent Pull Request - thank you!
Please consider making a few changes as per the comments.
Best wishes,
Paul

return SARA_R5_ERROR_OUT_OF_MEMORY;
}

sprintf(command, "%s=1", SARA_R5_FILE_SYSTEM_LIST_FILES);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment here that AT+ULSTFILE=1 returns the Remaining free FS space expressed in bytes.
(I was worried it was doing a full LS - and would overflow the minimumResponseAllocation)

free(response);
return err;
}
char *responseStart = strstr(response, "+ULSTFILE:");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

David's changes - which I like - move us away from hard-coded strings in the code. Is there an elegant way to append the ":" to SARA_R5_FILE_SYSTEM_LIST_FILES ? I.e. avoid hard-coding here; also avoid 'cheating' by defining SARA_R5_FILE_SYSTEM_LIST_FILES_RESPONSE (which includes the colon). Maybe create a new helper function which will append the colon to whatever command is passed?

}

size_t availableSpace;
responseStart += strlen("+ULSTFILE:"); // Move searchPtr to first char
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

@@ -5648,6 +5718,69 @@ SARA_R5_error_t SARA_R5::getFileBlock(const String& filename, char* buffer, size

return SARA_R5_ERROR_SUCCESS;
}
SARA_R5_error_t SARA_R5::getAvailableSize(size_t * size)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider renaming this to getAvailableFreeSpace

return err;
}

SARA_R5_error_t SARA_R5::ftpList(const String &dirName, char * response) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider renaming this to ftpListFiles

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Humm. Thinking about this more...
The response could be quite large, depending on how many files are in the system. You could easily overrun response if you're not careful...
(I know I cheated with this overload of getFileContents because there you can know in advance how much data is coming.)
So, here, it would be best to use a String - which can grow as required - or pass in the size of response so you can stop writing before you overrun. String gets my vote - same as this overload of getFileContents.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where possible I avoided the use of String in my changes to avoid heap problems, even though we're using an ESP32 so heap problems go away on deep sleep :)

I had originally changed a lot of existing methods to use char buffers too, but decided that was too much of an ask for a pull request so reverted them.

I'd be wary of using so many Strings on our Cortex M0+ based nodes which don't reboot after waking up.

You haven't found any problems with long running sketches?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi David (@dajtxx ),
We haven't noticed any problems but: we also tend to run this on ESP32; and we generally don't put the processor to sleep. In this case you're welcome to twist my arm and go with a char buffer, with size.
Cheers,
Paul

@@ -943,6 +943,10 @@ class SARA_R5 : public Print
SARA_R5_error_t connectFTP(void);
SARA_R5_error_t disconnectFTP(void);
SARA_R5_error_t ftpGetFile(const String& filename);
SARA_R5_error_t ftpChangeWorkingDirectory(const String& dirName);
SARA_R5_error_t ftpCreateDirectory(const String& dirName);
SARA_R5_error_t ftpList(const String& dirName, char * response);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

@@ -1011,6 +1015,8 @@ class SARA_R5 : public Print
SARA_R5_error_t getFileContents(String filename, char *contents); // OK for binary files. Make sure contents can hold the entire file. Get the size first with getFileSize.
SARA_R5_error_t getFileBlock(const String& filename, char* buffer, size_t offset, size_t length, size_t& bytes_read); // OK for binary files. Make sure buffer can hold the requested block size.

SARA_R5_error_t getAvailableSize(size_t * size); // Get available FS space
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

@PaulZC
Copy link
Collaborator

PaulZC commented Nov 15, 2023

Hi @kebab01 @dajtxx ,

Just giving you guys a nudge on this. We are working on a new library which will support both the SARA-R5 and LARA-R6. It would be nice to include your FTP code both here and there, if you can add the changes I requested.

Best wishes,
Paul

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants